Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Conversation

filipsekulic
Copy link
Contributor

Explanation

Fixed navigation through multiple unapproved transactions for ERC20 tokens.

Screenshots/Screencaps

Before

Before.mov

After

After.mov

Manual Testing Steps

  1. Navigate to the test dapp
  2. Connect your wallet
  3. Deploy the ERC20 token
  4. Click APPROVE TOKENS multiple times
  5. Navigate back to the wallet

@filipsekulic filipsekulic self-assigned this Dec 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@mirjanaKukic
Copy link
Contributor

Verified by QA

jpuri
jpuri previously approved these changes Dec 9, 2022
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good 👍

@PeterYinusa PeterYinusa added this to the v10.24.0 milestone Dec 12, 2022
@filipsekulic filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 991f5d8 to b8553f9 Compare December 13, 2022 13:40
@metamaskbot
Copy link
Collaborator

Builds ready [b8553f9]
Page Load Metrics (2233 ± 140 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint113176131147
domContentLoaded17142475217919594
load171431772233292140
domInteractive17142475217919594
Bundle size diffs
  • background: 0 bytes
  • ui: 1324 bytes
  • common: 0 bytes

highlights:

storybook

Comment on lines 215 to 245
const network = hexToDecimal(fullTxData.chainId);

const currentNetworkUnapprovedTxs = Object.keys(unapprovedTxs)
.filter((key) =>
transactionMatchesNetwork(
unapprovedTxs[key],
fullTxData.chainId,
network,
),
)
.reduce((acc, key) => ({ ...acc, [key]: unapprovedTxs[key] }), {});

const enumUnapprovedTxs = Object.keys(currentNetworkUnapprovedTxs);
const currentPosition = enumUnapprovedTxs.indexOf(
fullTxData.id ? fullTxData.id.toString() : '',
);

const totalTx = enumUnapprovedTxs.length;
const positionOfCurrentTx = currentPosition + 1;
const nextTxId = enumUnapprovedTxs[currentPosition + 1];
const prevTxId = enumUnapprovedTxs[currentPosition - 1];
const showNavigation = enumUnapprovedTxs.length > 1;
const firstTx = enumUnapprovedTxs[0];
const lastTx = enumUnapprovedTxs[enumUnapprovedTxs.length - 1];

const handleNextTx = (txId) => {
if (txId) {
dispatch(clearConfirmTransaction());
history.push(`${CONFIRM_TRANSACTION_ROUTE}/${txId}`);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is problematic but what I'm wondering is if there's an opportunity to improve the readability of our code by relocating and consolidating this logic and the 'confirm-transaction-base.container.js' files logic and move it into the <ConfirmPageContainerNavigation> component which appears to already be a functional component. Doing this would make that specific part of our code much more understandable and perhaps prevent needing to do this selector logic in multiple places.

Could you take a look into this and see how much effort it'll be to consolidate? Thanks @filipsekulic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @brad-decker! Good suggestion!
I refactored the code and you can see the changes here: f4b0938
However, there is a problem and you can see it in the attached screenshot, which is the reason why the pipeline is failing (e2e test).
This happens when there are multiple token approvals and when a user confirms one of them, the navigation disappears for some reason and throws the error (screenshot attached). I think the problem is in the confirm-page-container.component.js within the async componentDidMount, which the error message is also suggesting (...cancel all subscriptions and asynchronous tasks...).
I would appreciate if you could take a look and give your opinion on this one.

Screenshot 2022-12-15 at 14 01 42

@filipsekulic filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch 2 times, most recently from 4309ec0 to f4b0938 Compare December 15, 2022 08:21
const txData = useSelector(getTxData);

const network = hexToDecimal(txData.chainId);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Navigate transactions - should confirm and remove an unapproved transaction" e2e test is failing because: TimeoutError: Waiting for element to be located By(xpath, .//*[contains(concat(' ', normalize-space(./@class), ' '), ' confirm-page-container-navigation ')][(contains(string(.), '1 of 3') or contains(string(.), '1of3'))])

This probably means that during the e2e test run showNavigation is false, which probably means that enumUnapprovedTxs is an empty array. Looking at this new code for getting currentNetworkUnapprovedTxs compared to the old code in confirm-transaction-base.container.js, I notice that the chainId and network are here derived from the transaction but in confirm-transaction-base.container.js they are taken directly from state.metamask.

If this feature is working when running it manually in the browser, for both custom approvals and erc-20 token transfers, then the problem is likely in e2e test fixtures (unless there is something wrong with getting the chainId and network from the transaction?... I don't think so, but worth a double check)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right @danjm. Good catch!
I fixed that and also made a small change that solved the problem, so now txData is being passed from a parent component.
You can check it here: 1d005ae

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you have to change it so that txData is being passed in, instead of being selected? what problem did that solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user tries to confirm one of the transactions, it now shows the navigation, but doesn't get updated as it should - txData object being selected becomes empty, next tx cannot be seen on the next page and currentPosition is -1 as the currentTx cannot be found from enumUnapprovedTxs, because there is no txId from txData object which is empty at that moment.
Screencast:

Screen.Recording.2022-12-20.at.16.05.36.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

txData is derived from state in confirm-transaction-base.container.js which means we can get it here without drilling the prop down the line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it, but the problem with it is that somehow the txData gets empty when one of the transactions is confirmed.
You can see it in the above attached video where txDataFromSelector is actually the transaction data derived from state. I console.log-ged it in order to show this problem...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the code for getting txDataFromSelector? was it derived from state in the exact same way as it is in confirm-transaction-base.container.js?

@metamaskbot
Copy link
Collaborator

Builds ready [344d0df]
Page Load Metrics (2226 ± 369 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint961785195365175
domContentLoaded159754762209771370
load166454772226768369
domInteractive159754762209771370
Bundle size diffs
  • background: 0 bytes
  • ui: 726 bytes
  • common: 0 bytes

highlights:

storybook

@filipsekulic filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 344d0df to 1d005ae Compare December 20, 2022 13:05
@filipsekulic filipsekulic requested review from danjm and jpuri December 20, 2022 13:19
@metamaskbot
Copy link
Collaborator

Builds ready [1d005ae]
Page Load Metrics (2115 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint932121262713
domContentLoaded18052484209317182
load18052484211517986
domInteractive18052484209317182
Bundle size diffs
  • background: 0 bytes
  • ui: 722 bytes
  • common: 0 bytes

highlights:

storybook

jpuri
jpuri previously approved these changes Dec 21, 2022
const txData = useSelector(getTxData);

const network = hexToDecimal(txData.chainId);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

txData is derived from state in confirm-transaction-base.container.js which means we can get it here without drilling the prop down the line

@filipsekulic filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 1d005ae to 094315d Compare December 27, 2022 09:22
@metamaskbot
Copy link
Collaborator

Builds ready [094315d]
Page Load Metrics (1455 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97161122167
domContentLoaded11851888143017885
load11851976145519593
domInteractive11851888143017885
Bundle size diffs
  • background: 0 bytes
  • ui: 722 bytes
  • common: 0 bytes

highlights:

storybook

@filipsekulic filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 094315d to 8220055 Compare January 9, 2023 09:09
@metamaskbot
Copy link
Collaborator

Builds ready [453de3c]
Page Load Metrics (1627 ± 157 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint105168133168
domContentLoaded123323761624330159
load123323761627327157
domInteractive123323761624330159
Bundle size diffs
  • background: 0 bytes
  • ui: 497 bytes
  • common: 0 bytes

highlights:

storybook

@filipsekulic
Copy link
Contributor Author

@brad-decker
After today's conversation with @danjm and his suggestion, I managed to get the transaction id by using the useParams, so I removed the txData as the component's property and everything seems to work fine.
You can check the changes here: 453de3c

@brad-decker brad-decker merged commit fc83a1b into develop Jan 11, 2023
@brad-decker brad-decker deleted the fix/navigate-through-multiple-unapproved-approved-transactions branch January 11, 2023 15:01
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Users cannot navigate through multiple unapproved approve transactions
7 participants